Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SplitBrain implementation #1829

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Remove SplitBrain implementation #1829

merged 1 commit into from
Feb 12, 2025

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Feb 11, 2025

Remove deprecated SplitBrain implementation

  • ⚠️ keeping splitBrainThresholdSeconds in the GSLB spec and CRD because its removal would cause errors in existing GSLB instances. Field is described as deprecated now and is not used anywhere.
  • removing SplitBrainTTL seconds from CRD terratest resources
  • Remove SplitbrainEnabled from HelmChart and all its dependencies
  • Terratests
  • From Depresolver (including Heartbeat functionality etc.).
  • Cleaning Infoblox provider

In k8gb, we maintain deprecated code for the Infoblox provider – SplitBrain based on TXT records. Extensive changes in k8gb require me to refactor and maintain this code, which is why I am removing it.

for reviewers: Basically the core of change is infoblox.go and gslb.go, gslb_types.go + helmChart. the rest are tests, depresolver, mocks, data.

Copy link

netlify bot commented Feb 11, 2025

Deploy Preview for k8gb-preview ready!

Name Link
🔨 Latest commit 2f31832
🔍 Latest deploy log https://app.netlify.com/sites/k8gb-preview/deploys/67ac6cde25957e0008445170
😎 Deploy Preview https://deploy-preview-1829--k8gb-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k0da
k0da previously approved these changes Feb 11, 2025
ytsarev
ytsarev previously approved these changes Feb 11, 2025
@ytsarev
Copy link
Member

ytsarev commented Feb 11, 2025

It was working code but isolated to in-tree Infoblox provider code only. To my knowledge, nobody is using it in the wild. Externaldns-based providers are not affected by this change 👍

@kuritka kuritka dismissed stale reviews from ytsarev and k0da via bfc0b5f February 12, 2025 09:19
- from CRD
- Terratests
- CR, Spec definition
- HelmChart

Signed-off-by: Michal K <[email protected]>
@kuritka kuritka force-pushed the remove-split-brain-txt branch from bfc0b5f to 2f31832 Compare February 12, 2025 09:41
@kuritka kuritka requested review from ytsarev and k0da February 12, 2025 09:49
@kuritka
Copy link
Collaborator Author

kuritka commented Feb 12, 2025

@ytsarev , @k0da - please review one more time. In previous implementation was breaking change as I changed CRD (removed splitBrainThresholdSeconds), which causes potentially issues on already existing GSLBs as they could have defined splitBrainThresholdSeconds. After update splitBrainThresholdSeconds remains in CRD and scheme and gslb_types.go, otherwise is removed.

strategy:
    type: roundRobin
    splitBrainThresholdSeconds: 300
    dnsTtlSeconds: 30

@ytsarev
Copy link
Member

ytsarev commented Feb 12, 2025

@kuritka good catch. We need to track the splitBrainThresholdSeconds: field deprecation in consequent releases then. Could you create an Issue?

@kuritka
Copy link
Collaborator Author

kuritka commented Feb 12, 2025

@ytsarev #1831

@kuritka kuritka merged commit cb64ca4 into master Feb 12, 2025
27 checks passed
@kuritka kuritka deleted the remove-split-brain-txt branch February 12, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants